Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved hunting queries to workspace deployment saved searches #3469

Merged
merged 7 commits into from
Nov 29, 2021

Conversation

samikroy
Copy link
Contributor

Fixes # Removed actual tables from hunting queries and analytic rule.

Proposed Changes

  • Moved Solorigate Inventory check hunting query to workspace.json
  • Removed actual tables from Moved Solorigate Inventory
  • Removed actual tables from Solorigate Network Beacon Analytic rule

@samikroy
Copy link
Contributor Author

@javiersoriano & @v-maudan - please help in merging this as this validation is due to the files outside this PR.

@samikroy samikroy changed the title Patch 13 Removed actual tables from hunting queries and analytic rules Nov 17, 2021
@shainw
Copy link
Contributor

shainw commented Nov 18, 2021

@samikroy - I think the validation failure is properly failing, I am just not sure the validation should be configured the way it is. You are checking in JSON files to the Solutions folder. The validation indicates there should only be YAML files which is wrong and not only that the validation does not indicate which files should be YAML files.

Your checked in files:
Solutions/Training/Azure-Sentinel-Training-Lab/Artifacts/LinkedTemplates/alertRules.json
Solutions/Training/Azure-Sentinel-Training-Lab/Artifacts/LinkedTemplates/workspace.json
Solutions/Training/Azure-Sentinel-Training-Lab/Package/mainTemplate.json

Validation failure snip

[xUnit.net 00:00:53.44] Kqlvalidations.Tests.DetectionTemplateSchemaValidationTests.Validate_DetectionTemplates_AllFilesAreYamls [FAIL]
X Kqlvalidations.Tests.DetectionTemplateSchemaValidationTests.Validate_DetectionTemplates_AllFilesAreYamls [18ms]
Error Message:
All the files in detections and solution (Analytics rules) folder are supposed to end with .yaml
Expected: True
Actual: False

@v-maudan and @Amitbergman - I think you both worked on these validations and might be able to diagnose the bug in - https://github.com/Azure/Azure-Sentinel/blob/master/.script/tests/detectionTemplateSchemaValidation/DetectionTemplateSchemaValidationTests.cs

@samikroy
Copy link
Contributor Author

Thank you @shainw .
Request request your help @v-maudan & @Amitbergman here to proceed.
As this PR contains contains changes which will distinguish an actual incident being raised with current training solution solution .

@shainw
Copy link
Contributor

shainw commented Nov 18, 2021

@samikroy - Scratch that, just re-ran the checks and it passed. I think @petebryan fixed the issue in a recent PR - https://github.com/Azure/Azure-Sentinel/pull/3471/files. Looks like good to get approved now if @javiersoriano and @v-maudan agree.

@samikroy
Copy link
Contributor Author

Thank you @shainw.
Request @javiersoriano & @v-maudan your help here.

@javiersoriano
Copy link
Member

javiersoriano commented Nov 18, 2021

Hi @samikroy ,

We tried to leave the rules as similar as possible to the original ones, that's why we left the rest of the tables in the query even though they are not used. What is the benefit for the user if we remove the other tables?

I agree on moving the hunting query to workspace.json instead of having it in mainTemplate.json.

The rest of the changes are just cosmetic.

@samikroy
Copy link
Contributor Author

samikroy commented Nov 18, 2021

Agree @javiersoriano
The reason have tried to remove the actual tables is in case this solution is enabled in existing prod Sentinel then this rules might mix up a prod & test data during incident creation.
But, in case if you suggest otherwise, will revert the changes.

@javiersoriano
Copy link
Member

Got it. If that's the reason, I would change the name of the rule, so it clearly indicates that this is a test rule and that is seen in the incident as well.

That would involve changing screenshots and text in the guide though

@samikroy
Copy link
Contributor Author

Got it. If that's the reason, I would change the name of the rule, so it clearly indicates that this is a test rule and that is seen in the incident as well.

That would involve changing screenshots and text in the guide though

In addition, we might need to update this in the UI template too, but this this file is not a latest version (please point me to the correct file in that case)

https://github.com/Azure/Azure-Sentinel/blob/master/Solutions/Training/Azure-Sentinel-Training-Lab/Package/createUiDefinition.json

@javiersoriano
Copy link
Member

@samikroy the intention of the training lab is to be deployed in a clean workspace, and for sure not in a production workspace, so we are hesitant to change the rule name.

can you just leave the move of the savedSearch to workspace.json and remove the other changes so we can approve?

Thanks

@samikroy
Copy link
Contributor Author

@samikroy the intention of the training lab is to be deployed in a clean workspace, and for sure not in a production workspace, so we are hesitant to change the rule name.

can you just leave the move of the savedSearch to workspace.json and remove the other changes so we can approve?

Thanks

@javiersoriano - Done.
Thank you.

@samikroy samikroy changed the title Removed actual tables from hunting queries and analytic rules Moved hunting queries to workspace deployment saved searches Nov 18, 2021
@javiersoriano
Copy link
Member

Thanks @samikroy

All good from my side to approve, but I don't have permissions. @shainw or @v-maudan could you please approve?

@samikroy
Copy link
Contributor Author

Request @shainw & @v-maudan to approve and merge this.
Let me know for any queries.
Thank you.

@sreedharande sreedharande merged commit f16ab7b into Azure:master Nov 29, 2021
@samikroy samikroy deleted the patch-13 branch December 10, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants